-
-
Notifications
You must be signed in to change notification settings - Fork 125
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Android 2022.2.X+ Build Issues #205
Conversation
…sues. Remove Unity Hub version for Windows as there is no ability to pin hub versions due to Unity not maintaining old versions. Passing an old version is likely to break the build as the install script uses hard coded hashes that may not match the latest version
… setup and license acceptance. Added separate section to fix symlink issue.
&& . ~/.bashrc \ | ||
&& cd "${ANDROID_NDK_HOME}/toolchains/llvm/prebuilt/linux-x86_64/bin" \ | ||
# Symlink any file less than 64 bytes to the file name within the file. We assume there are no real files that small | ||
&& for f in $(find . -type f -size -64c); do target=$(cat $f) && echo "Making symlink $f -> $target" && rm $f && ln -s $target $f ; done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we using file size here for the loop? Is it to identify files that are symlinks?
If so, we can do so by using the -L
test to determine whether it is a symbolic link.
Example:
RUN echo "$version-$module" | grep -q -vP '^(2022.[2-9]|202[3-9]|20[3-9]).*android' \
&& exit 0 \
|| : \
&& . ~/.bashrc \
&& cd "${ANDROID_NDK_HOME}/toolchains/llvm/prebuilt/linux-x86_64/bin" \
&& for f in *; do \
if [ -L "$f" ]; then \
target=$(readlink "$f") && echo "Making symlink $f -> $target" && rm "$f" && ln -s "$target" "$f"; \
fi \
done
I haven't tried this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the files aren't actually symbolic links, they're a mix of ascii and binary files but inside they have the file name of the executable they should point to. We suspect hub isn't using the proper flags when unzipping so the symbolic links get lost hence the need for us to make the links ourselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly more manageable to separate that part into a different PR? Not sure though. And not preference from my side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the main point of the PR to fix the android build issues of 2022.2+ so I'm inclined to keep this as a single PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, makes sense!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not fully aware of the context or issues here so it'd be nice to have someone else review this as well, but this seems like a good change overall 👍 Thanks Andrew! :D
@@ -265,6 +265,7 @@ jobs: | |||
# Test # | |||
############ | |||
- name: Test project | |||
timeout-minutes: 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For other reviewers:
https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idtimeout-minutes
Default is 360, reducing it to 10 will most likely make it fail faster if necessary 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only a valid setting if you know the maximum amount a project would take to build. Useful for this repo. Perhaps also worth documenting in our docs for GHA?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general you don't really need this setting. I think what is happening is these big matrices of Unity test runs exceed some kind of rate limit for activation endpoint silently which causes it to hang. I just set it to 10 minutes because I saw every test run with our empty test project takes about 1 min so by 10 min it's probably hung and guaranteed to fail. This time is heavily driven by the actual tests and whether the library folder already exists. The activation errors are also generally rare in standard CI runs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are in fact a few cases where implementers of GameCI using GHA are waiting for their run and they report it times out after 5 hours not knowing the error.
Probably the fastest way to setup GameCI (in case anything goes wrong) is to set it up using a test project, and having that setting set to 5 minutes :) Just a suggestion though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me as well, though it's mostly important to verify that the changes do what you expect them to do.
&& . ~/.bashrc \ | ||
&& cd "${ANDROID_NDK_HOME}/toolchains/llvm/prebuilt/linux-x86_64/bin" \ | ||
# Symlink any file less than 64 bytes to the file name within the file. We assume there are no real files that small | ||
&& for f in $(find . -type f -size -64c); do target=$(cat $f) && echo "Making symlink $f -> $target" && rm $f && ln -s $target $f ; done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly more manageable to separate that part into a different PR? Not sure though. And not preference from my side.
|
||
# Install unity hub | ||
RUN choco install unity-hub --version=%hubVersion --no-progress -y | ||
RUN choco install unity-hub --no-progress -y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove the version flag? Is the argument no longer valid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It actually wasn't relevant from the start. Reading more into the readme for the chocolatey package, Unity doesn't store an archive of the older Unity hubs so it will always pull the latest version. The chocolatey script used a hard coded hash for each version to verify the download so putting an older version can cause errors during installs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They must have changed that. I wouldn't have added this param without it being documented.
Nice catch!
# Conflicts: # images/windows/hub/Dockerfile
# Conflicts: # images/windows/hub/Dockerfile
Changes
Checklist
Additional Context
Unity has updated the naming of its internal packages for Android. Google has also deprecated the bin/sdkmanager in favor of clitools bin/sdkmanager. Below are the path changes throughout the Unity Versions:
Android NDK
Android OpenJDK
Android SDK Platform Tools
Android SDK Command Line Tools
Scripts to Generate the Tables
Build Images (buildall.sh)
Pull Modules.json from each image (pullall.sh)
Generate Markdown (GenerateMD.ps1)